-
Notifications
You must be signed in to change notification settings - Fork 447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
scheduler: check daily job limit on each job, not just when choosing AV #3089
Conversation
I have spent some time testing this and there are a few issues. The first two seem to be issues with the previous PR #3024. I don't know why I didn't find them when I was testing previously.
In this PR the call to daily_quota_exceeded will always pass as the the number of tasks provided by the for loop are not taken into account. Either the DB needs to be updated on each iteration or a variable is required to store this value and taken into consideration by the function. |
I was doing the check on each call to get_app_version(). But it turns out this gets called during the job scan, before calling add_result_to_reply(), which is where n_jobs_today gets incremented. Solution:move the check to the loop (in sched_score.cpp) where add_result_reply() is called. Also: fix bad logic in work_needed() where a global check (on job limit in user project prefs) was being done inside a loop over resources.
Fixed now (I think) in this branch and the new dpa_punitive2 |
Still doesn't work. In the DB, max_jobs_per_day=1 and n_jobs_today=0 so I would expect 1 job but get 4. Here are relevant lines from the scheduler log.
Please could you do a pull request for dpa_punitive2, just click here. Please delete this comment first and I can merge immediately. |
I did a PR for dpa_punitive2, leaving the valid comment. |
In my testing this seemed to work. Remember that the daily limits are per (host, app version). Maybe it's sending jobs for other app versions. Set debug_version_select and debug_send. |
I rebuilt the scheduler from a fresh codebase and it now seems to work as advertised. I just want to leave it running on the dev project overnight before merging. |
Looks good. |
Fixes #3073